fix(rewrites): serve static files from public/ when rewrite target is a .html path#217
fix(rewrites): serve static files from public/ when rewrite target is a .html path#217yunus25jmi1 wants to merge 5 commits intocloudflare:mainfrom
Conversation
|
@southpolesteve @elithrar kindly review the changes. |
|
Kindly review the changes. @southpolesteve |
01837a4 to
098e14a
Compare
commit: |
|
/bigbonk review |
There was a problem hiding this comment.
Thanks for working on this — the use case (rewriting clean URLs to static .html files in public/) is a real gap. The overall approach of checking the filesystem after rewrites fail to match a route is reasonable. However, there are several issues that should be addressed before merging.
Summary of issues
-
Path traversal guard missing in dev server paths —
prod-server.tsusestryServeStatic()which has traversal protection, but the two dev-server paths (inindex.tsandapp-dev-server.ts) do not. While the rewrite destination is developer-controlled,cleanPathnamein the RSC entry can be influenced by request URLs after middleware processing. -
MIME type map duplication — The MIME type mapping is now maintained in three separate places with inconsistent entries. This will drift over time.
-
publicDirfallback is wrong forsrc/applayouts —path.dirname(appDir)produces/project/src/publicwhenappDiris/project/src/app. The Viterootis always passed from the call site, but the fallback is misleading and would be silently wrong if ever hit. -
String concatenation instead of
path.joinin generated RSC code — The generated code usespublicDir + cleanPathnamewhich doesn't normalize the path. -
deploy.ts(Cloudflare Workers Pages Router entry) not updated — Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others." The Cloudflare Workers entry (deploy.ts) has the same afterFiles/fallback rewrite logic without static file serving. This is likely fine because Cloudflare'sASSETSbinding serves static files at the platform level, but it should be explicitly acknowledged. -
Missing
navigationContextcleanup in RSC entry — When returning the static fileResponse,setHeadersContext(null)andsetNavigationContext(null)are not called, unlike all other early-return paths nearby.
| const allowedOrigins = config?.allowedOrigins ?? []; | ||
| // Compute the public/ directory path for serving static files after rewrites. | ||
| // appDir is something like /project/app or /project/src/app; root is the Vite root. | ||
| const publicDir = path.join(root ?? path.dirname(appDir), "public").replace(/\\/g, "/"); |
There was a problem hiding this comment.
The fallback path.dirname(appDir) is incorrect when appDir is /project/src/app — it would produce /project/src/public instead of /project/public. In practice root is always passed from the call site in index.ts, so this won't be hit at runtime. But the fallback is misleading. Consider throwing or logging a warning if root is not provided, rather than silently computing a wrong path.
|
Review submitted with REQUEST_CHANGES. Here's a summary of the feedback: Key issues identified:
|
|
I noticed in the issue that they were using a nested route, but we're only reproducing with a top-level route - it might be worth adding a nested one as well to ensure we're covering their scenario. |
|
@james-elicx As I go through the issues, I'm going to working on it after dinner. Kindly review the remaining PR. |
098e14a to
8f8e18a
Compare
|
@southpolesteve @james-elicx The fixes is pushed. Kindly review the changes. |
|
/bigbonk review |
There was a problem hiding this comment.
Good work on this PR — the use case is real (rewriting to static .html in public/) and the overall approach is sound. The shared MIME map is a nice improvement over the previous state. There are a few issues to address before merging.
Summary
-
includes(".")in prod-server.ts is too broad — The afterFiles and fallback checks inprod-server.tsstill useincludes(".")instead ofpath.extname(), which will trigger unnecessarytryServeStaticfilesystem lookups on URLs like/api/v2.0/data. The Pages Router dev server (index.ts) and the App Router dev server (generated RSC entry) both correctly usepath.extname(). This should be consistent. -
Pages Router dev server only checks static files at the very end — In
index.ts, the static file check runs after both afterFiles and fallback rewrites fail to match a route. Butprod-server.tscorrectly checks immediately after the afterFiles rewrite produces a.htmlpath. This means in dev, if afterFiles rewrites/footo/foo.htmland there's no route match, the code tries fallback rewrites before checking the filesystem. This isn't a correctness bug (the test passes because fallback rewrites won't match either), but it's a behavioral inconsistency with prod and does an unnecessary fallback rewrite attempt. Consider checking right after afterFiles rewrites resolve, matching the prod-server pattern. -
publicDirfallback is misleading — The comment correctly notes thatpath.dirname(appDir)is wrong forsrc/applayouts, but the fallbackroot ?? path.resolve(appDir, "..")is still present. Sincerootis always passed from the call site, this won't be hit in practice, but it would be cleaner to either throw or use a more defensive default.
| // If the rewritten path has a file extension, it may point to a static | ||
| // file in public/ (copied to clientDir during build). Try to serve it | ||
| // directly before falling through to SSR (which would return 404). | ||
| if (resolvedPathname.includes(".") && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
There was a problem hiding this comment.
includes(".") is too broad here — it triggers tryServeStatic on any URL containing a dot (e.g. /api/v2.0/data). Both index.ts and the generated RSC entry use path.extname() for this check. This should be consistent:
| if (resolvedPathname.includes(".") && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { | |
| if (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
| } | ||
| // Check if fallback targets a static file in public/ | ||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if (fallbackPathname.includes(".") && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { |
There was a problem hiding this comment.
Same includes(".") issue as above:
| if (fallbackPathname.includes(".") && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { | |
| if (path.extname(fallbackPathname) && tryServeStatic(req, res, clientDir, fallbackPathname, compress)) { |
packages/vinext/src/index.ts
Outdated
| // corresponds to a static file in the public/ directory (e.g., after a | ||
| // rewrite: /auth/no-access → /auth/no-access.html). Serve it directly so | ||
| // we don't hit the SSR 404 path. | ||
| const resolvedStaticPath = resolvedUrl.split("?")[0]; |
There was a problem hiding this comment.
This static file check runs at the very end, after fallback rewrites have already been tried and failed. In contrast, prod-server.ts checks right after afterFiles rewrites resolve a .html path (line 901) and again after fallback rewrites (line 923). The dev server should match the prod behavior:
- Check for static file right after afterFiles rewrites resolve (before
matchRoute) - Check again after fallback rewrites resolve
This avoids unnecessary fallback rewrite attempts and keeps dev/prod parity — something AGENTS.md explicitly calls out as important.
| // (e.g. /project/src/public instead of /project/public). | ||
| const publicDir = path.join(root ?? path.resolve(appDir, ".."), "public").replace(/\\/g, "/"); | ||
| // Build import map for all page and layout files | ||
| const imports: string[] = []; |
There was a problem hiding this comment.
The fallback root ?? path.resolve(appDir, "..") is documented as wrong for src/app layouts, but still present. Since root is always passed from index.ts, this code path won't execute in normal usage. But if someone adds a new call site without root, they'll get a silently wrong path. Consider making this defensive:
| const imports: string[] = []; | |
| const publicDir = path.join(root ?? (() => { console.warn("[vinext] generateRscEntry: root not provided, public/ path may be incorrect"); return path.resolve(appDir, ".."); })(), "public").replace(/\\/g, "/"); |
Or more simply, just assert that root is provided and throw if not.
| if (__extname) { | ||
| const __publicRoot = ${JSON.stringify(publicDir)}; | ||
| const __publicFilePath = __nodePath.resolve(__publicRoot, "." + cleanPathname); | ||
| // Path traversal guard — resolved path must stay inside public/ |
There was a problem hiding this comment.
Minor: this traversal guard uses hardcoded "/" as the separator. On Windows (if anyone runs the dev server there), path.resolve returns backslash-separated paths, so startsWith(__publicRoot + "/") would fail. Consider using __nodePath.sep instead:
if (__publicFilePath.startsWith(__publicRoot + __nodePath.sep)) {The index.ts version correctly uses path.sep. The generated RSC entry should match.
|
Review submitted. Here's a summary of what I flagged: Review Summary (REQUEST_CHANGES)5 inline comments across 3 files:
|
8f8e18a to
35cebb9
Compare
|
/bigbonk re-review please |
There was a problem hiding this comment.
Review Summary
Good work addressing the core issue — rewriting clean URLs to static .html files in public/ is a legitimate gap. The shared MIME map (mime.ts) is a nice consolidation that addresses the prior review's concern about duplication. The traversal guards are correctly implemented. Test coverage with both top-level and nested rewrites is solid — glad to see the nested case was added per @james-elicx's feedback.
However, there is one structural issue that should be addressed before merging, plus a couple of smaller items.
1. App Router dev/prod parity gap (important)
In app-dev-server.ts, the static file check only runs inside if (!match) (line 1793), meaning it fires after both afterFiles rewrites, matchRoute(), and fallback rewrites have all been attempted. But in prod-server.ts (lines 958, 980) and index.ts (lines 2984, 3029), the static file check runs immediately after each rewrite resolves, before route matching.
This means the App Router dev path:
- afterFiles rewrites
/foo→/foo.html matchRoute("/foo.html", routes)is called (always misses — no route matches.html)- Fallback rewrites are attempted (unnecessary for a
.htmlpath) - Then the
!matchblock finally checkspublic/
While functionally correct today (tests pass because .html paths don't match routes and fallback rewrites won't match either), this is a behavioral inconsistency that AGENTS.md explicitly calls out as important to avoid. The static file check should be inserted right after afterFiles rewrites resolve (between current lines 1773-1775) and after fallback rewrites resolve (between lines 1788-1789).
2. __mimeTypes object re-created on every request
In the generated RSC entry, ${JSON.stringify(MIME_TYPES)} creates a new object on every request that enters the !match + has-extension branch. This should be hoisted to module scope in the generated code.
3. Nit: redundant path.resolve in index.ts
The path.resolve(root, "public") call is duplicated — once for resolvedPublicDir and again inside the traversal check. Minor, but easy to clean up.
What looks good
- Shared MIME map in
mime.ts— clean consolidation, addresses prior duplication concern - Traversal guards —
path.resolve()+startsWith(dir + sep)pattern is correct in all three paths - Context cleanup —
setHeadersContext(null)+setNavigationContext(null)is correctly called before the static file return in the RSC entry (addresses prior review item) path.extname()usage — consistent across all three server paths (addresses prior review'sincludes(".")concern in prod-server)- Test coverage — both top-level and nested rewrites tested for both App Router and Pages Router
- Nested rewrite test — addresses @james-elicx's request to cover the nested route scenario from the original issue
| @@ -1781,6 +1791,27 @@ async function _handleRequest(request, __reqCtx, _mwCtx) { | |||
| } | |||
There was a problem hiding this comment.
Dev/prod parity issue: This static file check only runs inside if (!match) — i.e. after afterFiles rewrites, matchRoute(), and fallback rewrites have all been attempted. In contrast, prod-server.ts (line 958) and index.ts (line 2984) check for static files immediately after afterFiles rewrites resolve, before route matching.
The current flow in the App Router dev path:
- afterFiles rewrites
/foo→/foo.html(line 1773) matchRoute("/foo.html", routes)— always misses (line 1777)- Fallback rewrites attempted — unnecessary for
.html(line 1780) !matchblock finally checkspublic/(here)
The check should be inserted right after cleanPathname = __afterRewritten; (after line 1773), and again after cleanPathname = __fallbackRewritten; (after line 1788), matching the other server paths. This avoids unnecessary matchRoute and fallback rewrite calls and keeps dev/prod parity — something AGENTS.md explicitly calls out.
| const publicDir = root ? path.join(root, "public") : null; | ||
| // Build import map for all page and layout files | ||
| const imports: string[] = []; | ||
| const importMap: Map<string, string> = new Map(); |
There was a problem hiding this comment.
When root is not provided, publicDir becomes null and static file serving silently degrades. Since the only call site (index.ts line 2347) always passes root, this won't fire in practice. But the silent null makes it easy for a future caller to miss root and get no error.
Consider either making root required (since the single call site always provides it), or logging a warning when it's missing.
| if (__stat.isFile()) { | ||
| const __content = __nodeFs.readFileSync(__publicFilePath); | ||
| const __ext = __extname.slice(1).toLowerCase(); | ||
| const __mimeTypes = ${JSON.stringify(MIME_TYPES)}; |
There was a problem hiding this comment.
Minor: ${JSON.stringify(MIME_TYPES)} creates a new object literal on every request that enters this branch. Since this is generated code inside _handleRequest(), the __mimeTypes constant should be hoisted to module scope in the generated code (near the other generated constants like routes, __configRewrites, etc.).
packages/vinext/src/index.ts
Outdated
| const resolvedPublicDir = path.resolve(root, "public"); | ||
| const publicFilePath = path.resolve(root, "public", "." + afterFilesPathname); | ||
| if (publicFilePath.startsWith(resolvedPublicDir + path.sep) && fs.existsSync(publicFilePath) && fs.statSync(publicFilePath).isFile()) { |
There was a problem hiding this comment.
Nit: path.resolve(root, "public") is called twice — once for resolvedPublicDir and again inside the path.resolve(root, "public", ...) on the next line. You can reuse resolvedPublicDir:
| const resolvedPublicDir = path.resolve(root, "public"); | |
| const publicFilePath = path.resolve(root, "public", "." + afterFilesPathname); | |
| if (publicFilePath.startsWith(resolvedPublicDir + path.sep) && fs.existsSync(publicFilePath) && fs.statSync(publicFilePath).isFile()) { | |
| const resolvedPublicDir = path.resolve(root, "public"); | |
| const publicFilePath = path.resolve(resolvedPublicDir, "." + afterFilesPathname); | |
| if (publicFilePath.startsWith(resolvedPublicDir + path.sep) && fs.existsSync(publicFilePath) && fs.statSync(publicFilePath).isFile()) { |
Same pattern applies to the fallback block at lines 3030-3032.
| // If the rewritten path has a file extension, it may point to a static | ||
| // file in public/ (copied to clientDir during build). Try to serve it | ||
| // directly before falling through to SSR (which would return 404). | ||
| if (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
There was a problem hiding this comment.
Good — using path.extname() here is correct and consistent with the other server paths. The prior review flagged includes(".") as too broad; glad to see this was addressed.
|
Review submitted successfully with REQUEST_CHANGES. Here's a summary of what I flagged: Key issue:
Smaller items:
What's good:
|
35cebb9 to
424fe43
Compare
|
@elithrar @james-elicx @southpolesteve conflict resolved. Kindly review the PR and it's changes. |
|
For what it's worth, you don't need to keep tagging us |
There was a problem hiding this comment.
Review Summary
This PR addresses a real gap — rewriting clean URLs to static .html files in public/ is a legitimate use case and the 404 behavior was a bug. The shared MIME map in mime.ts is a good consolidation, traversal guards are correctly implemented, context cleanup is handled, and the test coverage (both top-level and nested rewrites, across both routers + a unit test for the generated entry) is solid.
However, there is one structural issue that's been flagged in all three prior automated reviews but remains unaddressed, plus a couple of smaller items.
Issues
-
App Router dev/prod parity gap (important) — The static file check in
app-dev-server.tsonly runs insideif (!match)at line 1837, meaning it fires after afterFiles rewrites,matchRoute(), and fallback rewrites have all been attempted. In contrast,index.ts(lines 2984, 3029) andprod-server.ts(lines 958, 980) check for static files immediately after each rewrite resolves, before route matching. This is functionally correct today but is a behavioral inconsistency that AGENTS.md explicitly calls out as important to avoid. -
__mimeTypesobject re-created on every request — In the generated RSC entry, the${JSON.stringify(MIME_TYPES)}expression at line 1851 creates a new object literal on every request that enters the!match+ has-extension branch. It should be hoisted to module scope in the generated code. -
Redundant
path.resolve(root, "public")calls — Inindex.ts,path.resolve(root, "public")is computed forresolvedPublicDirand then again in thepath.resolve(root, "public", ...)call on the next line. TheresolvedPublicDirvariable should be reused. Same pattern at lines 3030-3032. -
publicDirsilently becomesnull— Whenrootis not provided togenerateRscEntry(),publicDirisnulland static file serving silently degrades. The only call site always providesroot, but the silentnullmakes it easy for a future caller to miss. Consider makingrootrequired or logging a warning.
| } | ||
| } | ||
|
|
||
| if (!match) { |
There was a problem hiding this comment.
Dev/prod parity issue — This is the third review cycle flagging this, and it remains the most important structural issue in the PR.
This static file check only runs inside if (!match), i.e. after afterFiles rewrites → matchRoute() → fallback rewrites have all been attempted. But in index.ts (line 2984) and prod-server.ts (line 958), the check runs immediately after afterFiles rewrites resolve, before route matching.
Concrete flow difference in the App Router dev path:
- afterFiles rewrites
/foo→/foo.html(line 1811) matchRoute("/foo.html", routes)is called (line 1821) — always misses- Fallback rewrites attempted (line 1824) — unnecessary for a
.htmlpath !matchblock finally checkspublic/(here, line 1837)
The fix is to insert a static file check right after cleanPathname = __afterRewritten; (after line 1817) and again after cleanPathname = __fallbackRewritten; (after line 1832), matching the other two server paths. This avoids unnecessary matchRoute and fallback rewrite calls.
Per AGENTS.md: "When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as 'follow-ups' — fix them in the same PR." The parity requirement applies equally to behavioral consistency across the three server implementations.
| if (__stat.isFile()) { | ||
| const __content = __nodeFs.readFileSync(__publicFilePath); | ||
| const __ext = __extname.slice(1).toLowerCase(); | ||
| const __mimeTypes = ${JSON.stringify(MIME_TYPES)}; |
There was a problem hiding this comment.
This ${JSON.stringify(MIME_TYPES)} expression creates a new object literal on every request that enters this code path. Since this is generated code inside _handleRequest(), the constant should be hoisted to module scope in the generated code (near the other generated constants like routes, __configRewrites, etc.).
Something like:
// At module scope in the generated code
const __mimeTypes = { ... };Then reference __mimeTypes here instead of creating a new object each time.
| // appDir is something like /project/app or /project/src/app; root is the Vite root. | ||
| // We require `root` for correctness — path.dirname(appDir) is wrong for src/app layouts | ||
| // (e.g. /project/src/public instead of /project/public). | ||
| const publicDir = root ? path.join(root, "public") : null; |
There was a problem hiding this comment.
When root is not provided, publicDir becomes null and static file serving silently degrades (the generated code has if (__extname && null !== null) which is always false). The only call site (index.ts line 2347) always passes root, so this won't fire in practice. But the silent degradation makes it easy for a future caller to miss root and get no error or warning.
Consider either:
- Making
rootrequired (since the single call site always provides it), or - Logging a warning when it's missing, e.g.:
if (!root) console.warn("[vinext] generateRscEntry: root not provided, static file serving after rewrites will be disabled");
packages/vinext/src/index.ts
Outdated
| const afterFilesPathname = afterRewrite.split("?")[0]; | ||
| if (path.extname(afterFilesPathname)) { | ||
| const resolvedPublicDir = path.resolve(root, "public"); | ||
| const publicFilePath = path.resolve(root, "public", "." + afterFilesPathname); |
There was a problem hiding this comment.
path.resolve(root, "public") is computed for resolvedPublicDir on the line above, then path.resolve(root, "public", ...) is called again here instead of reusing resolvedPublicDir. Minor but easy to clean up:
| const publicFilePath = path.resolve(root, "public", "." + afterFilesPathname); | |
| const publicFilePath = path.resolve(resolvedPublicDir, "." + afterFilesPathname); |
Same pattern applies to the fallback block at line 3031.
packages/vinext/src/index.ts
Outdated
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if (path.extname(fallbackPathname)) { | ||
| const resolvedPublicDir = path.resolve(root, "public"); | ||
| const publicFilePath = path.resolve(root, "public", "." + fallbackPathname); |
There was a problem hiding this comment.
Same redundant path.resolve(root, "public") — reuse resolvedPublicDir here too:
| const publicFilePath = path.resolve(root, "public", "." + fallbackPathname); | |
| const publicFilePath = path.resolve(resolvedPublicDir, "." + fallbackPathname); |
|
Review submitted with REQUEST_CHANGES. Here's what I flagged: Key issue:
Smaller items:
What looks good:
|
…flare#199) Address code review feedback: - Extract shared MIME type map (server/mime.ts) eliminating 3x duplication - Add path traversal guard using resolve + startsWith pattern - Use path.extname() instead of string splitting - Use path.join/resolve in generated RSC entry instead of string concat - Clean up navigation/headers context before returning static response - Add nested route test (public/auth/no-access.html via rewrite)
424fe43 to
506d825
Compare
|
/bigbonk re-review this pr |
There was a problem hiding this comment.
Review
The fix is well-targeted — rewriting clean URLs to static .html files in public/ is a real use case (issue #199) and the 404 was a legitimate bug. The overall approach is sound: after rewrites resolve to a path with a file extension, check the filesystem before falling through to route matching / SSR.
Good things in this PR:
- Shared MIME map (
mime.ts) — clean consolidation that replaces the oldCONTENT_TYPESin prod-server and avoids drift - Traversal guards —
path.resolve()+startsWith(dir + sep)pattern is correct and consistent in all three server paths - Context cleanup —
setHeadersContext(null)+setNavigationContext(null)called before returning in the generated RSC entry path.extname()used consistently — avoids the overly broadincludes(".")that was in the old prod-server code- Dev/prod parity — all three server paths (index.ts, app-dev-server.ts, prod-server.ts) now check for static files immediately after each rewrite resolves, before route matching. I note the automated reviewer kept flagging a parity issue, but the current code is actually correct: in
app-dev-server.ts, the afterFiles static check (line 1824) runs beforematchRoute(line 1844), and the fallback static check (line 1857) runs before the fallbackmatchRoute(line 1874). - Test coverage — both top-level and nested rewrites tested across both routers, plus a unit test for the generated entry
deploy.tscorrectly left unchanged — Cloudflare's ASSETS binding serves static files at the platform level
I have a few items flagged inline — mostly minor, but one is a subtle correctness concern worth addressing.
| const afterFilesPathname = afterRewrite.split("?")[0]; | ||
| if (path.extname(afterFilesPathname)) { | ||
| const resolvedPublicDir = path.resolve(root, "public"); | ||
| const publicFilePath = path.resolve(resolvedPublicDir, "." + afterFilesPathname); |
There was a problem hiding this comment.
Nit: resolvedPublicDir is computed on line 2985, but then path.resolve(resolvedPublicDir, "." + afterFilesPathname) re-derives the same base. This is fine, but the "." + afterFilesPathname pattern relies on afterFilesPathname starting with / (which it does, since it comes from afterRewrite.split("?")[0] which is a rewrite destination). Worth a brief comment or assertion to document this assumption, since if a rewrite destination ever lacked a leading slash, the path resolution would be wrong.
Same applies to the fallback block at line 3031.
packages/vinext/src/index.ts
Outdated
| // Check if fallback targets a static file in public/ | ||
| const fallbackPathname = fallbackRewrite.split("?")[0]; | ||
| if (path.extname(fallbackPathname)) { | ||
| const resolvedPublicDir = path.resolve(root, "public"); |
There was a problem hiding this comment.
Minor: path.resolve(root, "public") is computed again here identically to the afterFiles block above (line 2985). Since both blocks are in the same function scope, you could hoist resolvedPublicDir above both blocks to avoid redundant computation on every request that hits fallback rewrites.
| const resolvedPublicDir = path.resolve(root, "public"); | |
| const publicFilePath = path.resolve(resolvedPublicDir, "." + fallbackPathname); |
(with resolvedPublicDir hoisted before the afterFiles block)
| if (!root) { | ||
| console.warn("[vinext] generateRscEntry: root not provided, static file serving after rewrites will be disabled"); | ||
| } | ||
| const publicDir = root ? path.join(root, "public") : null; |
There was a problem hiding this comment.
Good handling — the warning when root is missing is a clear improvement over the previous silent wrong fallback. The null propagation through the generated code (${JSON.stringify(null)} !== null → always false) cleanly disables the feature.
One thought: since root is effectively required for this feature to work and there's only one call site that always provides it, you could consider making it required (drop the ?) and let TypeScript enforce it. But the current approach is also fine for forward compatibility.
| const __afterPublicRoot = ${JSON.stringify(publicDir)}; | ||
| const __afterPublicFile = __nodePath.resolve(__afterPublicRoot, "." + cleanPathname); |
There was a problem hiding this comment.
Subtle issue: ${JSON.stringify(publicDir)} is evaluated at code-generation time and embedded as a string literal in the generated module. This means the value is the publicDir at the time generateRscEntry() was called. If someone moves or symlinks the public/ directory after the dev server starts, the embedded path would be stale. This is fine in practice (dev servers restart on config changes), but it's worth noting.
More importantly: the __nodePath.resolve(__afterPublicRoot, "." + cleanPathname) call assumes cleanPathname starts with /. Since it comes from __afterRewritten (a rewrite destination), this should always be true for well-formed Next.js config. But a malformed rewrite destination like foo.html (no leading slash) would resolve relative to the current working directory rather than public/, potentially reading files outside the project. The traversal guard on line 1828 would catch this (the resolved path wouldn't start with __afterPublicRoot), so it's not a security issue, but worth a brief comment.
| setNavigationContext(null); | ||
| return new Response(__afterContent, { status: 200, headers: { "Content-Type": __mimeTypes[__afterExt] ?? "application/octet-stream" } }); | ||
| } | ||
| } catch { /* file doesn't exist or not readable */ } |
There was a problem hiding this comment.
The bare catch {} silently swallows all errors, including permission errors (EACCES), too-many-open-files (EMFILE), etc. For a dev server, it would be helpful to log unexpected errors:
| } catch { /* file doesn't exist or not readable */ } | |
| } catch (e) { if (e?.code !== 'ENOENT') console.warn('[vinext] static file check failed:', e); } |
Same applies to the fallback block's catch at line 1871.
| // If the rewritten path has a file extension, it may point to a static | ||
| // file in public/ (copied to clientDir during build). Try to serve it | ||
| // directly before falling through to SSR (which would return 404). | ||
| if (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
There was a problem hiding this comment.
Good — using path.extname() here is correct and consistent with the other server paths. Clean improvement over the old includes(".") approach.
| export const MIME_TYPES: Record<string, string> = { | ||
| html: "text/html; charset=utf-8", | ||
| htm: "text/html; charset=utf-8", | ||
| css: "text/css", |
There was a problem hiding this comment.
Nice consolidation. One missing entry that's fairly common in Next.js apps: wasm: "application/wasm". Cloudflare Workers apps in particular use .wasm files. Not blocking, but worth adding if you're touching this file anyway.
|
Review submitted successfully. Here's a summary of what I flagged: Overall assessment: This PR is in good shape. The fix correctly addresses issue #199 and the implementation is solid across all three server paths. Key finding: The automated reviewer's persistent complaint about dev/prod parity in Inline comments posted (7 total):
|
|
Merge the PR to main. The PR is in a good state. |
Looks like Bonk has a few more comments before it's happy to approve |
Yeah, I observed this. |
# Conflicts: # packages/vinext/src/index.ts # packages/vinext/src/server/app-dev-server.ts
…add wasm MIME type, document leading-slash assumption
|
Addressed all bonk review items:
Re: making |
|
/bigbonk please re-review this pr |
There was a problem hiding this comment.
Review
This PR fixes a real gap — rewriting clean URLs to static .html files in public/ is a common Next.js pattern and the 404 was a legitimate bug. The approach is sound: after rewrites resolve to a path with a file extension, check the filesystem before falling through to route matching.
What looks good
- Shared MIME map (
mime.ts) — clean consolidation that replaces the old duplicatedCONTENT_TYPESin prod-server and prevents drift - Traversal guards —
path.resolve()+startsWith(dir + sep)pattern is correctly implemented in all three server paths - Context cleanup —
setHeadersContext(null)+setNavigationContext(null)properly called before returning the static file response in the generated RSC entry path.extname()used consistently across all three server paths- Dev/prod parity — all three server paths check for static files immediately after each rewrite resolves, before route matching. The afterFiles check fires before
matchRoutein all three paths, and the fallback check fires before fallbackmatchRoutein both app-dev-server and index.ts - Test coverage — both top-level and nested rewrites tested across both routers, plus a unit test for the generated entry code
- Non-ENOENT error logging in catch blocks — good for dev server diagnostics
Items
I have a few inline comments — one suggestion, a couple of observations, and a testing gap worth noting. Nothing blocking.
| const publicDir = root ? path.join(root, "public") : null; | ||
| // Build import map for all page and layout files | ||
| const imports: string[] = []; | ||
| const importMap: Map<string, string> = new Map(); |
There was a problem hiding this comment.
The publicDir is null when root isn't provided, and the generated code bakes in ${JSON.stringify(null)} !== null which is always false — effectively disabling the feature silently. The warning on lines 70-71 is good, but since the only call site (index.ts:2384) always passes root, consider making root required (drop the ?) so TypeScript enforces this at the call site. If a future caller forgets root, they'd get a type error instead of silently broken static file serving.
Non-blocking — the current code is safe.
| const __configRewrites = ${JSON.stringify(rewrites)}; | ||
| const __configHeaders = ${JSON.stringify(headers)}; | ||
| const __allowedOrigins = ${JSON.stringify(allowedOrigins)}; | ||
| const __mimeTypes = ${JSON.stringify(MIME_TYPES)}; |
There was a problem hiding this comment.
Good — __mimeTypes is correctly hoisted to module scope in the generated code. This was flagged in earlier reviews and has been properly addressed.
| if (__afterExtname && ${JSON.stringify(publicDir)} !== null) { | ||
| const __afterPublicRoot = ${JSON.stringify(publicDir)}; | ||
| // "." + cleanPathname works because rewrite destinations always start with "/"; | ||
| // the traversal guard below catches any malformed path regardless. | ||
| const __afterPublicFile = __nodePath.resolve(__afterPublicRoot, "." + cleanPathname); | ||
| if (__afterPublicFile.startsWith(__afterPublicRoot + __nodePath.sep)) { | ||
| try { | ||
| const __afterStat = __nodeFs.statSync(__afterPublicFile); | ||
| if (__afterStat.isFile()) { | ||
| const __afterContent = __nodeFs.readFileSync(__afterPublicFile); | ||
| const __afterExt = __afterExtname.slice(1).toLowerCase(); | ||
| setHeadersContext(null); | ||
| setNavigationContext(null); | ||
| return new Response(__afterContent, { status: 200, headers: { "Content-Type": __mimeTypes[__afterExt] ?? "application/octet-stream" } }); | ||
| } | ||
| } catch (__e) { if (__e?.code !== 'ENOENT') console.warn('[vinext] static file check failed:', __e); } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The static file check is correctly positioned — it runs immediately after cleanPathname = __afterRewritten (line 1917) and before matchRoute (which comes after this block). This matches the positioning in index.ts and prod-server.ts. The automated reviewer's persistent complaint about this being inside if (!match) was incorrect for the current code.
One observation: __afterPublicRoot on line 1922 re-evaluates ${JSON.stringify(publicDir)} which is a static string literal, so the JS engine will inline it. But it could be even cleaner to reference a single module-scope constant (like __mimeTypes) rather than embedding the same JSON.stringify(publicDir) literal in both the afterFiles and fallback blocks. Minor style nit.
|
|
||
| it("applies custom headers from next.config.js on API routes", async () => { | ||
| const res = await fetch(`${baseUrl}/api/hello`); | ||
| expect(res.headers.get("x-custom-header")).toBe("vinext-app"); |
There was a problem hiding this comment.
Good test coverage for both top-level and nested rewrites. One gap worth noting: there are no tests for fallback rewrites targeting static files. The code adds static file checking after both afterFiles and fallback rewrites, but only afterFiles is tested (the rewrite rules in the fixture config are all in afterFiles). Consider adding a fallback rewrite rule that maps to a .html file and a corresponding test — this would exercise the fallback static file path in all three servers.
Non-blocking since afterFiles is the common case from the original issue.
| // If the rewritten path has a file extension, it may point to a static | ||
| // file in public/ (copied to clientDir during build). Try to serve it | ||
| // directly before falling through to SSR (which would return 404). | ||
| if (path.extname(resolvedPathname) && tryServeStatic(req, res, clientDir, resolvedPathname, compress)) { |
There was a problem hiding this comment.
Clean improvement — path.extname() is more precise than the old includes('.') and is now consistent across all three server paths. The tryServeStatic() function already has its own traversal guard, so delegating to it here is the right call.
|
The review was submitted successfully. Here's a summary of what I posted: Review submitted on PR #217 with 5 inline comments:
Overall assessment: the PR is in good shape. The core fix is correct, the three server paths are properly synchronized, and the prior review items have been addressed. |
…rewrite static file tests
|
Addressed latest bonk review items:
Re All 213 app-router tests and 130/132 pages-router tests pass (2 pre-existing failures unrelated to this PR: "hydration proxy script" and "client bundle chunks"). |
|
/bigbonk re-review the pr please |
There was a problem hiding this comment.
Review
This PR correctly addresses issue #199 — rewriting clean URLs to static .html files in public/ is a common Next.js pattern and the 404 was a real bug. The approach is sound and well-structured across all three server paths.
What looks good
- Shared MIME map in
mime.ts— clean consolidation that eliminates prior duplication - Traversal guards are correctly implemented using
path.resolve()+startsWith(dir + sep)in all three paths - Context cleanup (
setHeadersContext(null)/setNavigationContext(null)) before returning in the RSC entry path.extname()used consistently across all server paths- Dev/prod parity — the static file check runs immediately after each rewrite resolves, before route matching, in all three server paths
- Test coverage — top-level, nested, and fallback rewrites tested for both App Router and Pages Router, plus a unit test for the generated entry code
- Non-ENOENT error logging in catch blocks — helpful for dev server diagnostics
Issues
I have two inline comments — one correctness concern and one code quality nit. Neither is blocking, but the first is worth addressing.
| // appDir is something like /project/app or /project/src/app; root is the Vite root. | ||
| // We need `root` for correctness — path.dirname(appDir) is wrong for src/app layouts | ||
| // (e.g. /project/src/public instead of /project/public). | ||
| const publicDir = root ? path.join(root, "public") : null; |
There was a problem hiding this comment.
Minor inconsistency: index.ts (line 3069) uses path.resolve(root, "public") while this uses path.join(root, "public"). Since this value is embedded as a static string in the generated code and then compared against __nodePath.resolve(...) results in the traversal guard (line 1923), path.resolve would be more defensively correct — it guarantees a fully normalized absolute path, whereas path.join doesn't normalize . or .. segments.
In practice this is fine because root comes from Vite's config.root which is always an absolute resolved path. But for consistency with index.ts and to make the traversal guard comparison more robust:
| const publicDir = root ? path.join(root, "public") : null; | |
| const publicDir = root ? path.resolve(root, "public") : null; |
| trailingSlash?: boolean, | ||
| config?: AppRouterConfig, | ||
| instrumentationPath?: string | null, | ||
| root?: string, |
There was a problem hiding this comment.
This function now has 10 positional parameters, which makes the call site at index.ts:2377-2384 hard to read (you have to count arguments to know what maps to what). Not something to fix in this PR, but worth noting as tech debt — a future refactor could bundle the later optional params into an options object.
For this PR specifically: since the only call site always provides root, making it required (dropping the ?) would be a stronger API choice. The null fallback + warning is safe but it means TypeScript won't catch a missing root at a future call site. Up to you — the current approach works.
|
Review submitted successfully. Here's a summary of what I posted: Overall assessment: The PR is in good shape. The fix correctly addresses issue #199 across all three server paths with proper security (traversal guards), cleanup (context nulling), and test coverage (top-level, nested, and fallback rewrites for both routers). Two inline comments:
|
Address bonk review: path.join → path.resolve for publicDir computation in app-dev-server.ts, matching the pattern used in index.ts. path.resolve guarantees a fully normalized absolute path, making the traversal guard comparison more robust.
|
Addressed the Bonk review item:
Re: the 10 positional parameters — acknowledged as technical debt, but not addressed in this PR as Bonk noted. All 213 app-router tests pass. Typecheck clean. |
Fixes #199.
Problem
When
next.configrewrites() map a clean URL to a static.htmlfile inpublic/(a common pattern for serving pre-built static pages like/auth/no-access→/auth/no-access.html), vinext was returning 404 because no Next.js route matches.htmlpaths.Root Cause
After
afterFilesrewrites resolve a path to*.html, the routing logic in all three server paths would find no matching app/pages route for the.htmlURL and return 404, without ever checking the filesystem'spublic/directory.Fix
Three server paths updated:
Pages Router dev (
packages/vinext/src/index.ts): After afterFiles/fallback rewrites, before the finalhandler()call, check if the resolved URL points to a file inpublic/and serve it directly.App Router dev (
packages/vinext/src/server/app-dev-server.ts): The generated RSC entry now checkspublic/for the rewritten pathname before rendering the 404 page. Added a new optionalrootparameter togenerateRscEntry()so the public directory path is embedded in the generated virtual module.Production server (
packages/vinext/src/server/prod-server.ts): After afterFiles rewrites and after fallback rewrites produce a path with a file extension,tryServeStatic()is called against the builtclientDir(which containspublic/files) before passing to SSR.Also added a
staticMimeType()helper inindex.tsto ensure correctContent-Typeheaders.Tests
app-router.test.tsandpages-router.test.ts:GET /static-html-page(rewritten →/static-html-page.html) returns 200 with correct HTML content andtext/htmlContent-Type.app-router.test.ts:generateRscEntry()embeds thepublic/path in the generated code.public/static-html-page.htmlfiles added to bothapp-basicandpages-basicfixtures;next.configrewrites updated.What async rewrites() flat array does
Per Next.js semantics, when
rewrites()returns a flat array, all rules go intoafterFiles. vinext already handles this correctly viaresolveNextConfig(). This PR fixes the serving of the rewritten destination when it's a static file.